Mirror cudf.pandas class-level monkeypatches onto the real type by galipremsagar · Pull Request #23001 · NVIDIA/cudf

galipremsagar · 2026-06-25T20:57:37Z

Split out of #22927 per review.

Problem

A class-level attribute write on a cudf.pandas proxy type — e.g. monkeypatch.setattr(pd.ExcelFile, "parse", fn) — was only applied to the proxy class. Code that runs under disable_module_accelerator() (such as the pandas fallback path of pd.read_excel) resolves attributes from the real class, so a patch applied only to the proxy was invisible to it.

Fix

Add _FastSlowProxyMeta.__setattr__/__delattr__ to mirror runtime class-level patches onto the underlying "slow" (real) type:

__setattr__ mirrors the assignment after translating the assigned value into "slow" space. A plain value is forwarded as-is. Re-assigning the proxy's pristine attribute for a name (which is exactly what monkeypatch.setattr / mock.patch.object save and re-assign on undo) translates to the slow type's pristine attribute for that name — restored if the slow type had one of its own, or removed if it didn't (leaving any inherited implementation visible). Proxy machinery (e.g. a saved pd.ExcelFile.parse, a _MethodProxy) unwraps to the slow object it delegates to.
__delattr__ mirrors a deletion as a deletion. Nothing is restored on delete.

The pristine state is a per-type map name -> (pristine proxy attribute, pristine slow class-dict entry) snapshotted once, when make_*_proxy_type finishes building the type (the same point mirroring is enabled via _fsproxy_mirror_slow_overrides). It is a fixed translation table, not runtime patch tracking: there is no stash of "what to put back", and undo works for any code that follows the standard save/patch/re-assign pattern (pytest monkeypatch, unittest.mock.patch.object, manual saves) because the saved value itself identifies the pristine state. The translation is what makes mirroring safe at all — the values readable off a proxy type live in proxy space, and forwarding e.g. the saved columns property or eval/query functions verbatim onto pandas.DataFrame would install cudf machinery on the real class (for columns this infinitely recurses on the fallback path).

cudf.pandas's own custom methods (DataFrame.eval/query) are installed via the new _setattr_fsproxy_no_mirror helper, which registers them as part of the proxy's pristine state without forwarding them to pandas.

Tests

Adds unit tests in cudf_pandas_tests/test_fast_slow_proxy.py covering: set/delete mirroring, monkeypatch round-trips (new attr, existing attr, nested, delattr), mock.patch.object (which saves the raw descriptor without resolving it), properties, plain data attributes, staticmethod/classmethod descriptor preservation, methods the slow type only inherits, and the no-mirror helper; plus an end-to-end test in test_cudf_pandas.py that patches/unpatches DataFrame.columns/eval and Series.str and checks real pandas is restored and functional.

Removes 14 now-passing xfails from the pandas-tests plugin (13× read_excel engine-selection tests that monkeypatch the engine, plus a monkeypatch-registered custom accessor). The attribution of these 14 to the proxy fix (vs the Excel-reader fixes remaining in #22927) was verified locally by running each removed xfail with the proxy fix in isolation.

Per review, split the proxy `__setattr__`/`__delattr__` monkeypatch-mirroring fix (and its tests in test_fast_slow_proxy.py) out of this PR; it now lives in NVIDIA#23001. The 14 pandas-tests it unblocks (read_excel engine selection + a monkeypatched custom accessor) are re-marked xfail here, since they require the proxy fix rather than the Excel-reader fixes. This PR keeps only the empty-column dtype and string-offset-width fixes.

coderabbitai · 2026-06-25T21:05:47Z

📝 Walkthrough

Walkthrough

The proxy metaclass now controls when class-level attribute changes propagate to slow types. The pandas wrapper installs DataFrame.eval and DataFrame.query without mirroring, and tests and the testing plugin were updated to match.

Changes

cuDF pandas proxy mirroring

Layer / File(s)	Summary
Mirror control in proxy metaclass `python/cudf/cudf/pandas/fast_slow_proxy.py`	`_FastSlowProxyMeta` now suppresses mirroring during construction, enables it after proxy types are built, and mirrors or restores slow-type attributes on class attribute changes.
Install DataFrame methods without mirroring `python/cudf/cudf/pandas/_wrappers/pandas.py`	`DataFrame.eval` and `DataFrame.query` are assigned through `_setattr_fsproxy_no_mirror` after importing the helper.
Validate mirroring and update expected failures `python/cudf/cudf_pandas_tests/test_fast_slow_proxy.py`, `python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py`	Tests cover class-attribute mirroring, deletion, teardown restoration, and the no-mirror helper, and the plugin removes expected-failure entries for affected tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested labels

improvement, non-breaking

Suggested reviewers

msarahan
TomAugspurger
bdice

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title accurately summarizes the main change: mirroring class-level monkeypatches from cudf.pandas proxies to the real type.
Description check	✅ Passed	The description directly explains the proxy mirroring fix, its mechanics, and tests, so it's clearly related to the changeset.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@python/cudf/cudf/pandas/fast_slow_proxy.py`:
- Around line 614-655: The teardown path in __setattr__ still forwards restored
proxy-local attributes back onto the real pandas class instead of restoring the
original slow implementation. Update __setattr__ on the fast/slow proxy class to
capture the pre-patch class attribute before type.__setattr__, and when
monkeypatch.undo() reassigns that same original proxy value (including plain
function/property-backed proxy members like DataFrame.eval and DataFrame.query),
treat it as a restore case by calling _fsproxy_restore_slow_attr(name) rather
than setattr(slow, name, value). Keep the existing handling for _MethodProxy,
_FastSlowAttribute, and _FastSlowProxy, but extend the restore detection to
cover the original proxy-local object identity.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 460c1729-97f2-4d18-b2b5-b7192cc9c92b

📥 Commits

Reviewing files that changed from the base of the PR and between 69dc079 and 0ff5ba4.

📒 Files selected for processing (4)

python/cudf/cudf/pandas/_wrappers/pandas.py
python/cudf/cudf/pandas/fast_slow_proxy.py
python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
python/cudf/cudf_pandas_tests/test_fast_slow_proxy.py

💤 Files with no reviewable changes (1)

python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py

vyasr

The implementation of setattr mirroring looks correct, but I'm not convinced that restoring via _fsproxy_restore_slow_attr represents the correct semantics. I'm guessing that this is meant to match pytest's monkeypatch fixture, but that's not how attribute setting/deleting should work in the general case unless I'm missing something.

copy-pr-bot · 2026-07-13T18:47:48Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

galipremsagar · 2026-07-13T18:48:46Z

/okay to test 835828b

galipremsagar · 2026-07-13T20:13:28Z

/okay to test c54ba21

…-attr mirroring Address review: __delattr__ now mirrors deletion as deletion (no restore), and the runtime patch stash (_fsproxy_slow_overrides/_fsproxy_restore_slow_attr) is gone. Instead, each proxy type snapshots its pristine public class attributes alongside the slow type's pristine class-dict entries when mirroring is enabled; __setattr__ translates assigned values into slow space: re-assigning the proxy's pristine attribute (what monkeypatch and mock.patch save and re-assign on undo) restores the slow type's pristine attribute, and proxy machinery unwraps to the slow object it delegates to. This also fixes real defects in the stash design's coverage: undo through unittest.mock (which saves the raw unresolved descriptor), patches of non-method attributes (properties, accessors, data attrs, cudf-installed attributes like DataFrame.columns/eval/query and Series.str, whose leak caused infinite recursion on the real type), classmethod/staticmethod descriptor preservation, and inherited methods no longer being copied into the slow type's dict on undo.

galipremsagar · 2026-07-18T14:15:44Z

/okay to test 72b8804

vyasr · 2026-07-20T22:38:19Z

/ok to test b17137b

vyasr

I'm having a bit of trouble following all the cases when we actually mirror. I left some suggestions for improvement, then I'll take a second pass and hopefully I'll grok all paths.

vyasr · 2026-07-21T02:42:03Z

+        # delegates to, so save/patch/re-assign cycles round-trip on the
+        # real type as well.
+        type.__setattr__(cls, name, value)
+        if not cls.__dict__.get("_fsproxy_mirror_slow_overrides", False):


Can this attribute ever not exist? We create on construction, so I think we should be safe to access it unconditionally (I assume this was AI-generated, AIs tend to always prefer these safe constructions because they don't validate the invariants).

Good instinct to question this — it flushed out a real bug. The attribute genuinely could be missing here, and the defensive lookup was silently papering over it: the flag was initialized in the metaclass __init__, but ABCMeta.__new__ (the ExcelFile/ExcelWriter proxies are built with metaclasses=(abc.ABCMeta,)) assigns __abstractmethods__ from inside __new__, which dispatches to this __setattr__ before __init__ ever runs. Switching to unconditional access made import cudf.pandas fail with AttributeError on ExcelWriter.

Fixed at the root: the flag is now initialized in _FastSlowProxyMeta.__new__ immediately after super().__new__(), so it exists before any cooperating metaclass can write class attributes, and the access here is unconditional as you suggested (with a comment documenting the ABCMeta ordering).

vyasr · 2026-07-21T02:42:39Z

+            # Mirroring is best-effort: translating a wrapped proxy instance
+            # can require a fast-to-slow conversion, which may itself fail;
+            # never let that escape an otherwise-successful assignment.
+            pristine = cls.__dict__.get("_fsproxy_pristine_attrs") or {}


Suggested change

pristine = cls.__dict__.get("_fsproxy_pristine_attrs") or {}

pristine = cls.__dict__.get("_fsproxy_pristine_attrs", {})

Is pristine ever not set? Don't we guarantee it by calling _enable_fsproxy_mirroring?

Superseded by the stronger form from your next comment: _fsproxy_pristine_attrs is guaranteed here, so it's now accessed unconditionally rather than defaulted.

Correct — it's guaranteed: _enable_fsproxy_mirroring sets _fsproxy_pristine_attrs before it flips _fsproxy_mirror_slow_overrides to True, and this code is only reachable when the flag is True. Now accessed unconditionally. Same reasoning applied to _fsproxy_slow_type (it's in the class namespace at types.new_class time for every class the two make_*_proxy_type factories build), so the slow is None early-return is gone as well.

vyasr · 2026-07-21T02:49:55Z

+                else:
+                    setattr(slow, name, entry[1])
+                return
+            slow_value = _mirror_value_to_slow(value, slow, name, pristine)


This is the only place _mirror_value_to_slow is used. Since many of its branches are early returns, I suggest we inline it so those returns can happen directly. I think it will also make the logic here a bit easier to track, jumping between the call site and the definition is a bit confusing given how much more complex the proxying logic is getting. We can also drop _MIRROR_SKIP entirely as a type.

Done — _mirror_value_to_slow is inlined into __setattr__ so each translation branch returns or mirrors directly at the call site, and _MIRROR_SKIP is gone. One nuance preserved from the helper: the classmethod/staticmethod descriptor-restore probe keeps its own inner try/except, so a raising __get__/__eq__ during the probe still falls back to mirroring the unwrapped function instead of aborting the mirror entirely.

vyasr · 2026-07-21T02:56:41Z

+                if entry[1] is _SLOW_ABSENT:
+                    if name in slow.__dict__:
+                        delattr(slow, name)


When do you hit this path? Shouldn't we avoid ever setting the attribute on the slow type in the first place?

This is the undo half of a mirror we very much want to make. The proxy's class dict is built from dir(slow_type), so it holds pristine entries for names the slow type only inherits — e.g. DataFrame.head lives on NDFrame, and pandas.DataFrame.__dict__ has no 'head'. When a user patches pd.DataFrame.head, the mirror must set head on pandas.DataFrame itself: that shadowing entry is the only way fallback code resolving through the real class sees the patch. This branch runs when the patch is undone (monkeypatch re-assigns the saved pristine proxy descriptor): the slow-space translation of "restore pristine" for a slow-inherited name is "delete the shadowing entry we added", making the inherited implementation visible again. The name in slow.__dict__ guard covers the case where the original mirror never landed (mirroring is best-effort), so there's nothing to delete. Expanded the code comment with this example — test_class_attr_inherited_method_monkeypatch_roundtrip exercises exactly this cycle.

Thanks for the detailed explanation, this makes sense now.

…ariants Initialize _fsproxy_mirror_slow_overrides in the metaclass __new__ rather than __init__: cooperating metaclasses can perform class-level attribute writes from their own __new__ (ABCMeta.__new__ assigns __abstractmethods__ for the ExcelFile/ExcelWriter proxies), dispatching to the mirroring __setattr__ before __init__ runs. With the flag guaranteed to exist, access it (and _fsproxy_slow_type/_fsproxy_pristine_attrs, both guaranteed once the flag is set by _enable_fsproxy_mirroring) unconditionally instead of via defensive cls.__dict__.get lookups. Also inline _mirror_value_to_slow into __setattr__ so its early returns read directly at the call site, dropping the _MIRROR_SKIP sentinel, and expand the comment on the _SLOW_ABSENT undo branch explaining why a mirrored patch for a slow-inherited attribute must be deleted on restore.

galipremsagar · 2026-07-21T13:25:28Z

Addressed the review in ec3a6a3: _mirror_value_to_slow/_MIRROR_SKIP are gone (logic inlined into __setattr__), and the defensive cls.__dict__.get lookups are replaced with unconditional attribute access. Making the accesses unconditional surfaced one real ordering bug the defaults had been hiding — ABCMeta.__new__ assigns __abstractmethods__ before the metaclass __init__ runs, breaking the ExcelFile/ExcelWriter proxies — fixed by initializing the mirroring flag in _FastSlowProxyMeta.__new__ instead (details in the thread).

Verified locally: test_fast_slow_proxy.py (51 passed), test_cudf_pandas.py (396 passed), the remaining top-level cudf_pandas_tests files (41 passed), and the 14 pandas-tests whose xfail entries this PR removes all pass; the full tests/io/excel/test_readers.py + tests/test_col.py files show no new raw failures vs the pre-change branch state.

galipremsagar · 2026-07-21T13:27:39Z

/okay to test ec3a6a3

copy-pr-bot · 2026-07-21T13:27:42Z

/okay to test ec3a6a3

@galipremsagar, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

galipremsagar · 2026-07-21T14:14:42Z

/okay to test 791beab

vyasr

OK, I think I've tracked all the paths here and they're doing the right things. Thanks for working through this.

vyasr · 2026-07-21T17:53:06Z

+                if entry[1] is _SLOW_ABSENT:
+                    if name in slow.__dict__:
+                        delattr(slow, name)


Thanks for the detailed explanation, this makes sense now.

galipremsagar · 2026-07-21T18:02:13Z

/merge

…IA#23001) Split out of NVIDIA#22927 per review. ## Problem A class-level attribute write on a cudf.pandas proxy type — e.g. `monkeypatch.setattr(pd.ExcelFile, "parse", fn)` — was only applied to the proxy class. Code that runs under `disable_module_accelerator()` (such as the pandas fallback path of `pd.read_excel`) resolves attributes from the *real* class, so a patch applied only to the proxy was invisible to it. ## Fix Add `_FastSlowProxyMeta.__setattr__`/`__delattr__` to mirror runtime class-level patches onto the underlying "slow" (real) type: - `__setattr__` mirrors the assignment after translating the assigned value into "slow" space. A plain value is forwarded as-is. Re-assigning the proxy's *pristine* attribute for a name (which is exactly what `monkeypatch.setattr` / `mock.patch.object` save and re-assign on undo) translates to the slow type's pristine attribute for that name — restored if the slow type had one of its own, or removed if it didn't (leaving any inherited implementation visible). Proxy machinery (e.g. a saved `pd.ExcelFile.parse`, a `_MethodProxy`) unwraps to the slow object it delegates to. - `__delattr__` mirrors a deletion as a deletion. Nothing is restored on delete. The pristine state is a per-type map `name -> (pristine proxy attribute, pristine slow class-dict entry)` snapshotted once, when `make_*_proxy_type` finishes building the type (the same point mirroring is enabled via `_fsproxy_mirror_slow_overrides`). It is a fixed translation table, not runtime patch tracking: there is no stash of "what to put back", and undo works for any code that follows the standard save/patch/re-assign pattern (pytest `monkeypatch`, `unittest.mock.patch.object`, manual saves) because the saved value itself identifies the pristine state. The translation is what makes mirroring safe at all — the values readable off a proxy type live in proxy space, and forwarding e.g. the saved `columns` property or `eval`/`query` functions verbatim onto `pandas.DataFrame` would install cudf machinery on the real class (for `columns` this infinitely recurses on the fallback path). cudf.pandas's own custom methods (`DataFrame.eval`/`query`) are installed via the new `_setattr_fsproxy_no_mirror` helper, which registers them as part of the proxy's pristine state without forwarding them to pandas. ## Tests Adds unit tests in `cudf_pandas_tests/test_fast_slow_proxy.py` covering: set/delete mirroring, monkeypatch round-trips (new attr, existing attr, nested, `delattr`), `mock.patch.object` (which saves the raw descriptor without resolving it), properties, plain data attributes, `staticmethod`/`classmethod` descriptor preservation, methods the slow type only inherits, and the no-mirror helper; plus an end-to-end test in `test_cudf_pandas.py` that patches/unpatches `DataFrame.columns`/`eval` and `Series.str` and checks real pandas is restored and functional. Removes 14 now-passing xfails from the pandas-tests plugin (13× `read_excel` engine-selection tests that monkeypatch the engine, plus a monkeypatch-registered custom accessor). The attribution of these 14 to the proxy fix (vs the Excel-reader fixes remaining in NVIDIA#22927) was verified locally by running each removed xfail with the proxy fix in isolation. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23001

galipremsagar requested a review from a team as a code owner June 25, 2026 20:57

galipremsagar requested review from brandon-b-miller and mroeschke June 25, 2026 20:57

github-actions Bot assigned galipremsagar Jun 25, 2026

github-actions Bot added Python Affects Python cuDF API. cudf.pandas Issues specific to cudf.pandas labels Jun 25, 2026

github-project-automation Bot added this to cuDF Python Jun 25, 2026

GPUtester moved this to In Progress in cuDF Python Jun 25, 2026

galipremsagar mentioned this pull request Jun 25, 2026

Fix cudf.pandas Excel reader failures (empty-column dtype, string offset width, proxy monkeypatch) #22927

Merged

3 tasks

coderabbitai Bot reviewed Jun 25, 2026

View reviewed changes

Comment thread python/cudf/cudf/pandas/fast_slow_proxy.py

vyasr requested changes Jun 30, 2026

View reviewed changes

Comment thread python/cudf/cudf/pandas/fast_slow_proxy.py Outdated

galipremsagar added bug Something isn't working non-breaking Non-breaking change labels Jul 13, 2026

galipremsagar requested a review from vyasr July 15, 2026 13:43

galipremsagar added 2 commits July 17, 2026 01:52

split up

31804de

galipremsagar force-pushed the cudf-pandas-mirror-monkeypatches branch from c54ba21 to 49a88e4 Compare July 17, 2026 01:54

galipremsagar changed the base branch from main to release/26.08 July 17, 2026 01:54

Merge branch 'release/26.08' into cudf-pandas-mirror-monkeypatches

72b8804

Merge branch 'release/26.08' into cudf-pandas-mirror-monkeypatches

b17137b

github-actions Bot assigned vyasr Jul 20, 2026

vyasr requested changes Jul 21, 2026

View reviewed changes

Merge branch 'release/26.08' into cudf-pandas-mirror-monkeypatches

791beab

vyasr approved these changes Jul 21, 2026

View reviewed changes

galipremsagar added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Jul 21, 2026

rapids-bot Bot merged commit a537b77 into NVIDIA:release/26.08 Jul 21, 2026
127 checks passed

github-project-automation Bot moved this from In Progress to Done in cuDF Python Jul 21, 2026

coderabbitai Bot mentioned this pull request Jul 21, 2026

Fix Parquet stats pinned host copy lifetime #23219

Merged

4 tasks

coderabbitai Bot mentioned this pull request Jul 21, 2026

Forward-merge release/26.08 into main #23382

Merged

josephine-wolf-oberholtzer added this to cuDF Python Aug 12, 2026

josephine-wolf-oberholtzer moved this to Done in cuDF Python Aug 12, 2026

	pristine = cls.__dict__.get("_fsproxy_pristine_attrs") or {}
	pristine = cls.__dict__.get("_fsproxy_pristine_attrs", {})

Uh oh!

Conversation

galipremsagar commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Tests

Uh oh!

coderabbitai Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested labels

Suggested reviewers

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

copy-pr-bot Bot commented Jul 13, 2026

Uh oh!

galipremsagar commented Jul 13, 2026

Uh oh!

galipremsagar commented Jul 13, 2026

Uh oh!

galipremsagar commented Jul 18, 2026

Uh oh!

vyasr commented Jul 20, 2026

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galipremsagar commented Jul 21, 2026

Uh oh!

galipremsagar commented Jul 21, 2026

Uh oh!

copy-pr-bot Bot commented Jul 21, 2026

Uh oh!

galipremsagar commented Jul 21, 2026

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galipremsagar commented Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

galipremsagar commented Jun 25, 2026 •

edited

Loading

coderabbitai Bot commented Jun 25, 2026 •

edited

Loading